Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding hx-headers-* server-side processing to hx-headers #36

Merged
merged 4 commits into from
Oct 16, 2023
Merged

Adding hx-headers-* server-side processing to hx-headers #36

merged 4 commits into from
Oct 16, 2023

Conversation

bingzer
Copy link
Contributor

@bingzer bingzer commented Oct 11, 2023

Hi @khalidabuhakmeh ,

I'm suggesting the addition of hx-headers-* support to Htmx.Net, which will enable developers to set custom HTTP headers for htmx requests. This enhancement aligns with the documentation https://htmx.org/attributes/hx-headers.

With this feature, you can now define headers like this:

<button type="button"
        hx-post
        hx-page="./Headers"
        hx-headers-Key1="value1"
        hx-headers-Key2="value2"
        hx-swap="innerHTML"
        hx-target="#post-result">Click Me</button>

or

<button type="button"
        hx-post
        hx-page="./Headers"
        hx-headers="new { Key1 = "value1", Key2 = "value2" }"
        hx-swap="innerHTML"
        hx-target="#post-result">Click Me</button>

This could also be useful to add RequestVerificationToken

@inject IAntiforgery antiforgery;
...

<button type="button"
        hx-post
        hx-page="./Headers"
        hx-headers-RequestVerificationToken="@antiforgery.GetAndStoreTokens(HttpContext).RequestToken"
        hx-swap="innerHTML"
        hx-target="#post-result">Click Me</button>
...

I want to add unit tests but I don't see Htmx.TagHelper.Tests project. Should the unit tests go under Htmx.Tests project?

I'm excited to hear your feedback on this proposal and whether it aligns with the project's objectives. Thank you for considering this improvement. I also have Sample code in the Sample project. Please take a look and let me now what you think.

@khalidabuhakmeh
Copy link
Owner

Yes, typically the unit tests go under Htmx.Tests.

My only concern with this approach is it could break existing HTMX implementations. If there already is an hx-headers attribute. Could you add a test that checks to see if the attribute already exists and if the value on the other end is already a string. If it is, we should default to staying with the string value.

@khalidabuhakmeh
Copy link
Owner

Keep in mind. We probably want to avoid hi-jacking or colliding with any of the htmx attributes. So while I like the hx-headers-* style I am not sure about taking over the hx-headers attribute as that could cause folks headaches.

What are your thoughts?

@bingzer
Copy link
Contributor Author

bingzer commented Oct 11, 2023

Yes, I agree! I was going to suggest removing hx-headers while keeping the hx-headers-*. I can do that.

So to clarify, if there's already value in hx-headers, I would then just use the existing hx-headers?

@bingzer
Copy link
Contributor Author

bingzer commented Oct 12, 2023

Hi @khalidabuhakmeh

I made some changes:

  • Removed hx-headers
  • Added Unit Tests for HtmxHeadersTagHelper
  • Updated Sample usage

Please review at your convenience.

@khalidabuhakmeh
Copy link
Owner

khalidabuhakmeh commented Oct 12, 2023 •
edited
Loading

I was curious about keys that are commonly used, and it seems like this approach should be OK. I was concerned that folks might want a key without dashes, but that doesn't seem to be something outside the convention.

https://en.wikipedia.org/wiki/List_of_HTTP_header_fields

Would you mind trying out some of the longer keys and see what that would look like? (Your code looks like it would handle it fine but might be worth adding to the sample).

@khalidabuhakmeh khalidabuhakmeh marked this pull request as ready for review October 12, 2023 16:39
@khalidabuhakmeh khalidabuhakmeh changed the title Adding hx-headers-* and hx-headers={ anonymous object } Adding hx-headers-* server-side processing to hx-headers Oct 12, 2023
@bingzer
Copy link
Contributor Author

bingzer commented Oct 12, 2023

Randomly picked a few standard and non standard Header Fields:

   <button type="button"
           hx-post
           hx-page="./Headers"
           hx-headers-X-Forwarded-Host="en.wikipedia.org:8080"
           hx-headers-X-Front-End-Https="on"
           hx-headers-X-Forwarded-For="client1, proxy1, proxy2"
           hx-headers-Upgrade-Insecure-Requests="1"
           hx-headers-X-LongCustom-Header="Stuff"
           hx-swap="innerHTML"
           hx-target="#post-result">
       Click me (List_of_HTTP_header_fields)
   </button>

I was able to see the requests were sent by the browser:
image

@khalidabuhakmeh khalidabuhakmeh merged commit d25882b into khalidabuhakmeh:main Oct 16, 2023
1 check passed
@khalidabuhakmeh
Copy link
Owner

Thanks, this looks great. I'll try to get a new version of this released.

@bingzer
Copy link
Contributor Author

bingzer commented Oct 16, 2023

Woot! Thanks 👍

@khalidabuhakmeh khalidabuhakmeh mentioned this pull request Oct 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bingzer @khalidabuhakmeh